Revise best-practices.md #569
Conversation
building/tooling/best-practices.md
Outdated
|
|
||
| 1. A Docker container is created | ||
| 2. The Docker container is run with the correct arguments | ||
| 3. The Docker container is destroyed |
There was a problem hiding this comment.
| 3. The Docker container is destroyed | |
| 3. The Docker container is destroyed. |
There was a problem hiding this comment.
Hmm, why is a period needed here?
There was a problem hiding this comment.
Ah, mistake on my part on mobile. The full context wasn't showing to mark all the steps, but a period should be used after an item in a list if every item is a sentence on its own. See https://apastyle.apa.org/style-grammar-guidelines/lists/numbered and https://www.microsoft.com/en-us/microsoft-365-life-hacks/writing/punctuating-bullet-point-lists.
There was a problem hiding this comment.
I can't add a comment to it, but L49 should read "Tooling runs as a one-off, short-lived Docker container:"
There was a problem hiding this comment.
L61 should read 'Therefore, it's fine if the code that runs at build-time is (relatively) slow.'
There was a problem hiding this comment.
L120 should read 'The reason "slim" variants are smaller is that they'll have fewer features.'
There was a problem hiding this comment.
L140 should read 'Distributions that use the apk package manager (such as Alpine) should use the --no-cache flag when using apk add to install packages:'
There was a problem hiding this comment.
L217 should read 'Its Dockerfile starts with a stage (given the name build) that installs those packages (via apk add) and then installs the libraries (via bundle install):'
tasxatzial
force-pushed
the
best-practices
branch
from
04cc11f to
156afb4
Compare
building/tooling/best-practices.md
Outdated
| - Be faster to deploy | ||
| - Reduce costs for us | ||
| - Improve startup time of each container | ||
| - Improve the startup time of each container |
There was a problem hiding this comment.
The prior line doesn't read "Reduce the cost". I don't think this needs "the" added.
There was a problem hiding this comment.
Sounds reasonable. Removed.
tasxatzial
force-pushed
the
best-practices
branch
from
156afb4 to
9cd0de5
Compare
tasxatzial
force-pushed
the
best-practices
branch
from
9cd0de5 to
57d3756
Compare
building/tooling/best-practices.md
Outdated
|
|
||
| 1. A Docker container is created | ||
| 2. The Docker container is run with the correct arguments | ||
| 3. The Docker container is destroyed |
There was a problem hiding this comment.
L61 should read 'Therefore, it's fine if the code that runs at build-time is (relatively) slow.'
building/tooling/best-practices.md
Outdated
|
|
||
| 1. A Docker container is created | ||
| 2. The Docker container is run with the correct arguments | ||
| 3. The Docker container is destroyed |
There was a problem hiding this comment.
L120 should read 'The reason "slim" variants are smaller is that they'll have fewer features.'
building/tooling/best-practices.md
Outdated
|
|
||
| 1. A Docker container is created | ||
| 2. The Docker container is run with the correct arguments | ||
| 3. The Docker container is destroyed |
There was a problem hiding this comment.
L140 should read 'Distributions that use the apk package manager (such as Alpine) should use the --no-cache flag when using apk add to install packages:'
building/tooling/best-practices.md
Outdated
|
|
||
| 1. A Docker container is created | ||
| 2. The Docker container is run with the correct arguments | ||
| 3. The Docker container is destroyed |
There was a problem hiding this comment.
L217 should read 'Its Dockerfile starts with a stage (given the name build) that installs those packages (via apk add) and then installs the libraries (via bundle install):'
Co-authored-by: András B Nagy <[email protected]>
|
Ready for review Edit: added the first suggestion (a period should be used after an item in a list if every item is a sentence on its own) |
Co-authored-by: Victor Goff <[email protected]>
tasxatzial
changed the title
Co-authored-by: Isaac Good <[email protected]>
|
@BNAndras Whenever you're able, your review is still needed. |
5 participants
No description provided.